Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update computation of cdn_ocn for use in dynamics #771

Merged
merged 1 commit into from
Oct 14, 2022
Merged

Update computation of cdn_ocn for use in dynamics #771

merged 1 commit into from
Oct 14, 2022

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Oct 12, 2022

PR checklist

PASS cheyenne_intel_qcchk_gx1_144x1_medium_qc_qcchk compare cice.422117fc5d.221008-085008 -1 -1 -1
PASS cheyenne_pgi_qcchk_gx1_144x1_medium_qc_qcchk compare cice.422117fc5d.221008-085008 -1 -1 -1
PASS cheyenne_gnu_qcchk_gx1_144x1_medium_qc_qcchk compare cice.422117fc5d.221008-085008 -1 -1 -1

  • How much do the PR code changes differ from the unmodified code?

    • bit for bit
    • different at roundoff level
    • more substantial

  • Does this PR create or have dependencies on Icepack or any other models?

    • Yes
    • No

  • Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.

    • Yes
    • No

  • Does this PR add any new test cases?

    • Yes
    • No

  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)

    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No

  • Please provide any additional information or relevant details below:

  • Compute and used cdn_ocn on U, E, and N cell locations as needed for dynamics.

  • Add halo updates in dynamics before calling grid_average. This doesn't change answers, but is the safe thing to do in general. Performance does not seem to be affected.

@apcraig
Update computation of cdn_ocn for use in dynamics
f01afd9 
- Compute and used cdn_ocn on U, E, and N cell locations as needed for dynamics.
- Add halo updates in dynamics before calling grid_average.  This doesn't
  change answers, but is the safe thing to do in general.  Performance does
  not seem to be affected.
@apcraig
Copy link
Contributor Author

apcraig commented Oct 12, 2022

Please check if this seems correct. This closes #737.

@eclare108213
Copy link
Contributor

I'm confused about when/how variable form drag is invoked -- this does not seem to be documented well in Icepack. E.g. formdrag=T should turn on the formdrag parameterization, which adjusts the ocean drag coefficients for various ice characteristics (in icepack_atmo.F90, not the most intuitive place), but if it is not turned on then Cdn_ocn is set to a constant value. There is also an option calc_dragio, which when true apparently varies the ocean drag coefficient in some other way (and might conflict with formdrag?). If neither of these parameterizations is turned on, then Cdn_ocn should be constant (=dragio=5.36e-3), and so for the B grid I would not expect the answers to change. Do they?

@apcraig
Copy link
Contributor Author

apcraig commented Oct 12, 2022

The B cases do changes answers. Looking closer at output, I think we're getting a round off level change in the cdn_ocn when we average a constant field to another grid. On the first timestep, the only differences in the log file are in KE, ice spped, and arwt ocn heat flux and those are roundoff different in the global sum. The difference then grows. So I think that's consistent with our expectation and it's OK.

I don't have any insight into the underlying computation of cdn_ocn, just focused on the grid location. Happy to make other changes, in documentation or otherwise, as needed.

@eclare108213
Copy link
Contributor

So on the B-grid it's a roundoff change... Is the "more substantial" tickmark above for C-grid simulations?

I found the calc_dragio documentation and will make an issue to make the documentation more transparent (calc_dragio isn't mentioned, which is why I didn't find it initially). In fact it's also a constant value (as long as the ice roughness and upper ocean grid spacing are constants), but it is computed from those parameters rather than assigned.

@apcraig
Copy link
Contributor Author

apcraig commented Oct 12, 2022

I called it "more substantial" because it will produce more-than-roundoff different results for non-constant cdn_ocn fields. Partly I didn't realize that cdn_ocn is a mostly constant. But I still think "more substantial" is the correct characterization, even if our tests happen to mostly (or always) use cdn_ocn=constant fields. We are using an "S" mapping to C grid too, so that should map a constant field to a constant field plus roundoff even for C grids.

@apcraig

This comment was marked as off-topic.

Sign in to view
@eclare108213

This comment was marked as off-topic.

Sign in to view
@apcraig

This comment was marked as off-topic.

Sign in to view
@apcraig apcraig merged commit 0447b9e into CICE-Consortium:main Oct 14, 2022
@apcraig apcraig mentioned this pull request Oct 21, 2022
Closed
@DeniseWorthen DeniseWorthen mentioned this pull request Nov 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eclare108213 eclare108213 eclare108213 approved these changes

@dabail10 dabail10 Awaiting requested review from dabail10

@JFLemieux73 JFLemieux73 Awaiting requested review from JFLemieux73

@phil-blain phil-blain Awaiting requested review from phil-blain

Assignees
No one assigned
Labels
Dynamics Priority: High Type: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@apcraig @eclare108213